Skip to content

Add question status data to user progress endpoint#760

Open
sjd210 wants to merge 6 commits intomainfrom
hotfix/progress-question-status
Open

Add question status data to user progress endpoint#760
sjd210 wants to merge 6 commits intomainfrom
hotfix/progress-question-status

Conversation

@sjd210
Copy link
Contributor

@sjd210 sjd210 commented Feb 24, 2026

Converts QuestionPageDTO to ContentSummaryDTO earlier in the method, and set question state according to the already calculated question part correctness loop.

This is done so that the "My progress" page can display the status of "Most recently answered questions"/"Oldest unsolved questions" on their ALVIs.

Also requires isaacphysics/isaac-react-app#2006, as status can otherwise take up too much space on small ALVIs such as these.

@codecov
Copy link

codecov bot commented Feb 24, 2026

Codecov Report

❌ Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.94%. Comparing base (5d9c0c5) to head (b5562d3).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...m/cl/dtg/segue/api/managers/StatisticsManager.java 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #760      +/-   ##
==========================================
- Coverage   39.95%   39.94%   -0.02%     
==========================================
  Files         546      546              
  Lines       23901    23907       +6     
  Branches     2873     2873              
==========================================
  Hits         9550     9550              
- Misses      13444    13450       +6     
  Partials      907      907              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

}

// Convert QuestionPageDTO to ContentSummaryDTO and calculate completion state
ContentSummaryDTO contentSummaryDTO = contentSummarizerService.extractContentSummary(questionPageDTO);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a good understanding of how expensive this method is? I think this will add a major overhead on this endpoint, since mapping (which extractContentSummary does under the hood) is an involved process requiring making a new object (which then needs garbage collecting in all but 5 cases here).
But MapStruct is less bad than Orika, so I would also not be surprised to find my memory of mapping being horrifyingly costly and slow is no longer true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine you've got a better understanding of such things than I do, even now with MapStruct 🤷‍♀️

We could certainly avoid this either way by creating a parallel set of queues for each of question pages and question statuses, and in fact I nearly did in the first place except that this then didn't work with the current implementation of incompleteQuestionPages. It would either need another list of question statuses, or to limit its size within this loop rather than only doing it afterwards. The latter seems like a good idea in general - I'll see how that comes out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just tried moving the mapping out of the loop (now on this branch) and done some experimenting with my local data.

I see no major difference (no more than within regular variance) in the time taken to load the progress data a) without these changes, b) with these changes and the mapping inside the loop, and c) with these changes and the mapping outside the loop on my main test account with 1000s of question attempts. This isn't particularly thorough testing, but it suggests that MapStruct is efficient enough that it no longer makes much difference.

I think my original implementation is cleaner code, but if we still want to be careful about the mapping then the changes now present should be safer in that regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants